feat(kotlin-sdk): add L1 invitation create/claim JNI bridge + DIP-13 invitation persistence - #4240
feat(kotlin-sdk): add L1 invitation create/claim JNI bridge + DIP-13 invitation persistence#4240bfoss765 wants to merge 5 commits into
Conversation
…tation persistence Adds the Android JNI + kotlin-sdk bridge for the existing (iOS-shipped) DIP-13 invitation FFI (create_invitation / claim_invitation), plus durable invitation persistence. - rs-unified-sdk-jni: JNI createInvitation/claimInvitation marshalers over platform_wallet_create_invitation / platform_wallet_claim_invitation; wire the on_persist_invitations_fn trampoline (tramp_persist_invitations). - kotlin-sdk: IdentityNative externs, IdentityRegistration create/claim wrappers, NativePersistenceBridge invitation dispatch, and PlatformWalletPersistenceHandler overrides that declare the INVITATIONS capability (0x02) gating funded invite creation. - Room: new invitations table (InvitationEntity/InvitationDao), DashDatabase v7 -> v8 with MIGRATION_7_8 and exported 8.json. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🔍 Review in progress — actively reviewing now (commit 9328609) |
📝 WalkthroughWalkthroughAdds DIP-13 DashPay invitation creation and claiming APIs, JNI implementations, invitation persistence callbacks, Room entities and DAO operations, a Room version 7-to-8 migration, and ChainLock fallback handling for invitation claims. ChangesDashPay invitations
Estimated code review effort: 5 (Critical) | ~90 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/InvitationDao.kt (1)
15-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
@Upsert;@Insert(REPLACE)is delete-then-insert, not an in-place overwrite.
INSERT OR REPLACEdeletes the conflicting row before inserting. That is harmless today (no table referencesinvitations, and every column is supplied on each call), but it makes the KDoc's "overwrites the row in place" inaccurate and would silently cascade if a child table is ever added.♻️ Proposed change
-import androidx.room.OnConflictStrategy -import androidx.room.Insert +import androidx.room.Upsert @@ - `@Insert`(onConflict = OnConflictStrategy.REPLACE) + `@Upsert` suspend fun upsert(invitation: InvitationEntity)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/InvitationDao.kt` around lines 15 - 23, Update InvitationDao.upsert to use Room’s `@Upsert` annotation instead of `@Insert`(onConflict = OnConflictStrategy.REPLACE), and remove the now-unused conflict-strategy import. Keep the existing suspend method and InvitationEntity parameter unchanged.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt (2)
314-344: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSilently dropped
inviterUsernamewheninviterIdentityIdis omitted.Only the "id present, username missing" direction is validated. If a caller passes
inviterUsernamewithoutinviterIdentityId, neither this method,IdentityNative.createInvitation, nor the Rustplatform_wallet_create_invitationcore (which gatesInviterInfopurely oninviter_identity_id.is_null()) ever reads the username — it's silently discarded and the caller gets a plain funding voucher with no error. Also, the KDoc here omits the "(only used when inviterIdentityId is non-null)" clarification thatIdentityNative.ktalready documents for the same parameter.♻️ Proposed fix
inviterIdentityId?.let { require(it.size == 32) { "inviterIdentityId must be 32 bytes, got ${it.size}" } require(inviterUsername != null) { "inviterUsername is required when inviterIdentityId is provided" } } + require(inviterIdentityId != null || inviterUsername == null) { + "inviterIdentityId is required when inviterUsername is provided" + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt` around lines 314 - 344, Update createInvitation to reject any non-null inviterUsername when inviterIdentityId is null, while preserving the existing requirement that an identity ID requires a username. Add the corresponding KDoc clarification that inviterUsername is only used when inviterIdentityId is non-null, and ensure the validation occurs before the invitation is created.
325-404: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo unit tests included for the new
createInvitation/claimInvitationvalidation logic.Boundary checks (amount/index/nowUnix ranges, inviter id/username coupling, uri/keys emptiness) are cheap to unit test and easy to regress silently later.
As per coding guidelines: "Keep pull requests focused, link related issues, include tests, and complete the pull request template."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt` around lines 325 - 404, Add unit tests covering validation in createInvitation and claimInvitation, including invalid amountDuffs, fundingAccountIndex, nowUnix, inviterIdentityId size/username coupling, blank uri, empty keys, and negative identityIndex. Verify each invalid input fails before invoking the corresponding IdentityNative method, while preserving successful validation behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt`:
- Around line 314-344: Update createInvitation to reject any non-null
inviterUsername when inviterIdentityId is null, while preserving the existing
requirement that an identity ID requires a username. Add the corresponding KDoc
clarification that inviterUsername is only used when inviterIdentityId is
non-null, and ensure the validation occurs before the invitation is created.
- Around line 325-404: Add unit tests covering validation in createInvitation
and claimInvitation, including invalid amountDuffs, fundingAccountIndex,
nowUnix, inviterIdentityId size/username coupling, blank uri, empty keys, and
negative identityIndex. Verify each invalid input fails before invoking the
corresponding IdentityNative method, while preserving successful validation
behavior.
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/InvitationDao.kt`:
- Around line 15-23: Update InvitationDao.upsert to use Room’s `@Upsert`
annotation instead of `@Insert`(onConflict = OnConflictStrategy.REPLACE), and
remove the now-unused conflict-strategy import. Keep the existing suspend method
and InvitationEntity parameter unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f8611829-42f6-4767-b020-a618f6b23ba3
📒 Files selected for processing (10)
packages/kotlin-sdk/sdk/schemas/org.dashfoundation.dashsdk.persistence.DashDatabase/8.jsonpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/IdentityNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativePersistenceBridge.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabase.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/InvitationDao.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/InvitationEntity.ktpackages/rs-unified-sdk-jni/src/identity.rspackages/rs-unified-sdk-jni/src/persistence.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The JNI marshalling and Room invitation schema are generally consistent, but three in-scope blockers remain. Invitation funding-index persistence can silently succeed without writing the security-critical address pool, clear-all omits the new invitation table, and coroutine cancellation can discard the only bearer URI after the native operation has already funded the voucher.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:507-511: Fail when the invitation funding account is missing
This handler now advertises the complete invitation-creation persistence capability, but the address-pool callback still reports success when its account lookup fails. For account type 5 (`IdentityInvitation`), this pool is the only state restored after restart that marks the exported voucher funding index as used; the invitation metadata row stores the index but is not used to rebuild the address pool. Consequently, `return@stage` lets the pre-broadcast `store()` and `flush()` gate succeed while dropping the funding-index update, allowing the same bearer voucher key to be selected again after restart. The Swift backend deliberately reports failure for this exact type-5 lookup miss. Throwing during staged transaction replay makes `onChangesetEnd` return nonzero and aborts creation before broadcast.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/services/DataManager.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/services/DataManager.kt:48-53: Clear-all leaves persisted invitations behind
The new `invitations` table has no foreign key to `wallets`, so deleting wallet and account rows does not remove its contents. No other `DataManager` category clears this table, which means both the wallet-category clear and the UI's "Clear All Data" operation leave sent-invitation metadata on disk despite promising to delete all local records. Those rows can become visible again if the same wallet ID is imported later.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt:333: Coroutine cancellation can lose the bearer URI after funding the voucher
`gate.op` runs the blocking JNI call through `withContext(Dispatchers.IO)`. As `TeardownGate` itself documents, cancellation while native code is running can be observed only when `withContext` dispatches the completed result back, causing that result to be discarded. The native invitation operation may already have broadcast the asset lock, persisted its record, waited for proof, and generated the bearer URI by then; JNI cannot observe the Kotlin cancellation. Because Room intentionally stores no URI or voucher key, and the Android bridge exposes neither URI regeneration nor invitation reclaim, the caller can lose the only shareable credential after funds have moved. Invitation creation needs a non-cancellable completion/handoff contract or a recovery API that can regenerate or retrieve the URI from durable state.
| inviterUsername: String? = null, | ||
| nowUnix: Long, | ||
| coreSignerHandle: Long, | ||
| ): CreatedInvitation = gate.op { |
There was a problem hiding this comment.
🔴 Blocking: Coroutine cancellation can lose the bearer URI after funding the voucher
gate.op runs the blocking JNI call through withContext(Dispatchers.IO). As TeardownGate itself documents, cancellation while native code is running can be observed only when withContext dispatches the completed result back, causing that result to be discarded. The native invitation operation may already have broadcast the asset lock, persisted its record, waited for proof, and generated the bearer URI by then; JNI cannot observe the Kotlin cancellation. Because Room intentionally stores no URI or voucher key, and the Android bridge exposes neither URI regeneration nor invitation reclaim, the caller can lose the only shareable credential after funds have moved. Invitation creation needs a non-cancellable completion/handoff contract or a recovery API that can regenerate or retrieve the URI from durable state.
source: ['codex']
There was a problem hiding this comment.
Fixed in 638430b: createInvitation now checks ensureActive() up front (cancellation before the native call is still honored) and then runs the JNI call and result construction under withContext(NonCancellable), so a completed result can no longer be discarded when withContext dispatches back to a cancelled caller — the bearer URI is always delivered once the voucher may have been funded. The cancellation contract is documented in the method KDoc.
There was a problem hiding this comment.
Resolved in 638430b — Coroutine cancellation can lose the bearer URI after funding the voucher no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Follow-up on this thread, since the CI failure it caused belongs with it: fixed in 9328609.
The NonCancellable fix in 638430b turned CI red — GateCoverageLintTest > everyHandleBorrowingSuspendFunIsGated. Not a real ungated borrow: createInvitation is still fully inside gate.op { }. The fix moved it off an expression body (it now needs a require(...) preamble and the withContext(NonCancellable) wrapper), and the lint only recognised gating in expression-body position — = gate.op {. createInvitation is the SDK's only block-bodied handle borrower out of 51, so that shape had simply never come up.
I didn't allowlist it. It isn't exempt, and an ALLOWLIST entry would blind the lint to a future refactor that drops the bracket from the one method where losing it costs a bearer credential after funds have moved — which is the opposite of what this guard is for. Taught the scan both body shapes instead: expression bodies keep the exact opener rule, and a block body must open its bracket before the first JNI call. That still rejects every real defect shape — no bracket, a plain withContext(Dispatchers.IO) borrow, a bracket opened after the native call, and a native call leaked outside the bracket — while permitting the argument validation and non-cancellable delivery wrapper the cancellation contract requires.
Validated against all 51 handle-borrowing suspend funs plus synthetic cases for each of those four violation shapes before running the suite. 186 tests, 0 failures — same count CI reported, with the one failure gone.
One thing I deliberately left alone: your separate nitpick about the cancellation contract sitting inside the @param fundingAccountIndex block tag is still open — I kept this commit to the CI fix.
…e claim (dashpay#4240) A voucher islock is signed at creation, so by claim time (minutes-to-hours later, possibly across a testnet platform-4.1 quorum rotation) Drive may reject it with InvalidInstantAssetLockProofSignatureError ("try chain asset lock proof instead"). claim_invitation now recovers exactly as the register/top-up paths do: - reconstruct_asset_lock_proof / assemble_asset_lock_proof return a ReconstructedProof { primary, chain_fallback }. When the link carried an islock AND the funding tx is already chain-locked, a ChainLock proof over the SAME credit output is built as chain_fallback. - The primary proof is submitted through submit_with_cl_height_retry. If it is an IS proof rejected with is_instant_lock_proof_invalid(), the ChainLock fallback is resubmitted over the same outpoint. If no fallback exists (tx not yet chain-locked), AssetLockNotChainLocked surfaces a clear retry signal. Adds unit tests for the three assemble outcomes (chainlock-only, islock+chainlocked→fallback, islock+not-chainlocked→no-fallback). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The L1-invite change added the INVITATIONS capability (0x02) via onPersistInvitationUpsert/Removal, so persistenceCapabilitiesBits() is now 0xbf, not 0xbd. Update the stale assertions (the test still expected the pre-invitation bitmask and asserted INVITATIONS absent). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
changed the title
Review fixes for the three blockers plus one functional minor: - PlatformWalletPersistenceHandler.onPersistAccountAddressPoolEntry: a missing IdentityInvitation (type tag 5) funding account now THROWS instead of silently skipping the address-pool write, mirroring Swift's persistAccountAddresses. That pool is the only restart-surviving record of a used voucher funding index; the throw fails the staged round so the Rust pre-broadcast durability gate aborts before funds move, preventing bearer-voucher key reuse. Other account types keep the tolerant skip. - DataManager: clear the FK-less `invitations` table in the WALLETS category so both the category clear and clearAll() actually remove sent-invitation metadata instead of letting it resurface on re-import. - IdentityRegistration.createInvitation: run the native call under withContext(NonCancellable) (after an explicit ensureActive() gate) so coroutine cancellation observed while withContext dispatches back can no longer discard the only bearer URI after the voucher was already funded. Cancellation contract documented in the KDoc. - createInvitation now rejects inviterUsername without inviterIdentityId (previously silently discarded by the native layer) and the KDoc says so. Verified: :sdk:compileDebugKotlin + :sdk:testDebugUnitTest (persistence handler / DataManager / IdentityRegistration tests) pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed 638430b addressing all three blockers plus the functional CodeRabbit note:
Skipped as low-value per CodeRabbit's own triage: the Verified with @coderabbitai review |
|
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Carried forward: the stale-islock → ChainLock orchestration test gap remains valid, while the three prior Kotlin blockers are fixed at exact head. Latest delta: the persistence failure propagation and non-cancellable bearer-URI handoff are correct, but both security-sensitive paths lack direct regression tests, and the new cancellation paragraph is attached to the wrong KDoc tag. No in-scope correctness blocker remains.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— security-auditor (completed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— general (completed)
🟡 3 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs:483-523: Test the stale-islock resubmission branch
The fallback tests only verify that proof assembly produces an InstantSend primary and an optional ChainLock fallback. They never make the first Platform submission return `InvalidInstantAssetLockProofSignatureError` or observe the second submission, so regressions in `is_instant_lock_proof_invalid`, proof selection, or no-fallback/error handling would still pass. Add an injectable submission seam and assert that the InstantSend proof is attempted first, the ChainLock retry uses the same voucher outpoint and key set, a missing fallback returns `AssetLockNotChainLocked`, and unrelated errors propagate without resubmission.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:511-527: Regression-test missing invitation-account failure propagation
The production fix is correct, but no test calls `onPersistAccountAddressPoolEntry` for a missing type-5 account. This callback has two distinct failure paths: standalone execution must return nonzero immediately, while a bracketed changeset initially stages successfully and must fail from `onChangesetEnd(success = true)`, roll back the transaction, and propagate the nonzero result through the FFI persistence round. Add coverage for both paths and preserve the intentionally tolerated skip for missing non-invitation account types; these are the exact semantics Rust's pre-broadcast durability gate relies on.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt:365-389: Regression-test the non-cancellable bearer-URI handoff
The fix correctly keeps the gated JNI call, blob validation, URI decoding, and `CreatedInvitation` construction inside `withContext(NonCancellable)`, but no `createInvitation` test or injectable native-call seam exercises cancellation during the blocking call. Add an internal create-invitation native-call dependency, cancel after the fake native call starts, then release it and verify that caller-visible code receives the exact outpoint and URI and that the teardown-gate lease is released. This protects the method-level guarantee from a future refactor that narrows or removes the protected region.
| let identity = match submit_with_cl_height_retry(settings, |s| { | ||
| placeholder.put_to_platform_and_wait_for_response_with_private_key( | ||
| &self.sdk, | ||
| asset_lock, | ||
| primary.clone(), | ||
| &voucher_priv.0, | ||
| identity_signer, | ||
| settings, | ||
| s, | ||
| ) | ||
| .await | ||
| .map_err(PlatformWalletError::Sdk)?; | ||
| }) | ||
| .await | ||
| { | ||
| Ok(identity) => identity, | ||
| Err(e) if is_instant_lock_proof_invalid(&e) => { | ||
| let Some(chain_proof) = chain_fallback else { | ||
| // The islock was rejected but the funding tx is not yet | ||
| // chain-locked, so no ChainLock proof can be built. Surface a | ||
| // clear retry signal rather than the raw consensus error. | ||
| return Err(PlatformWalletError::AssetLockNotChainLocked( | ||
| "invitation islock proof was rejected by Platform (stale — quorum \ | ||
| rotated or no longer recent) and the funding transaction is not yet \ | ||
| chain-locked, so no ChainLock fallback is possible; retry once the \ | ||
| funding block is chain-locked" | ||
| .to_string(), | ||
| )); | ||
| }; | ||
| tracing::warn!( | ||
| "invitation IS-lock proof rejected by Platform on claim; retrying with a \ | ||
| ChainLock proof over the same funding outpoint" | ||
| ); | ||
| submit_with_cl_height_retry(settings, |s| { | ||
| placeholder.put_to_platform_and_wait_for_response_with_private_key( | ||
| &self.sdk, | ||
| chain_proof.clone(), | ||
| &voucher_priv.0, | ||
| identity_signer, | ||
| s, | ||
| ) | ||
| }) | ||
| .await | ||
| .map_err(PlatformWalletError::Sdk)? | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Test the stale-islock resubmission branch
The fallback tests only verify that proof assembly produces an InstantSend primary and an optional ChainLock fallback. They never make the first Platform submission return InvalidInstantAssetLockProofSignatureError or observe the second submission, so regressions in is_instant_lock_proof_invalid, proof selection, or no-fallback/error handling would still pass. Add an injectable submission seam and assert that the InstantSend proof is attempted first, the ChainLock retry uses the same voucher outpoint and key set, a missing fallback returns AssetLockNotChainLocked, and unrelated errors propagate without resubmission.
source: ['claude', 'codex']
| ) ?: run { | ||
| // Mirror Swift `persistAccountAddresses`: a missing account is | ||
| // tolerated for every type EXCEPT `IdentityInvitation` (type | ||
| // tag 5). That account is registered at wallet setup and its | ||
| // address pool is the ONLY state restored after a restart that | ||
| // marks a voucher funding index as used (the invitation | ||
| // metadata row stores the index but never rebuilds the pool) — | ||
| // silently dropping the write would let the same bearer | ||
| // voucher key be selected again. Throwing fails the staged | ||
| // round, so `onChangesetEnd` (or the standalone `guarded` | ||
| // path) returns nonzero and the Rust pre-broadcast durability | ||
| // gate aborts invitation creation before funds move. | ||
| check((accountTypeTag.toInt() and 0xFF) != ACCOUNT_TYPE_IDENTITY_INVITATION) { | ||
| "IdentityInvitation account missing for wallet ${walletId.toHex()}; " + | ||
| "refusing to drop the voucher funding-index address-pool write" | ||
| } | ||
| return@stage |
There was a problem hiding this comment.
🟡 Suggestion: Regression-test missing invitation-account failure propagation
The production fix is correct, but no test calls onPersistAccountAddressPoolEntry for a missing type-5 account. This callback has two distinct failure paths: standalone execution must return nonzero immediately, while a bracketed changeset initially stages successfully and must fail from onChangesetEnd(success = true), roll back the transaction, and propagate the nonzero result through the FFI persistence round. Add coverage for both paths and preserve the intentionally tolerated skip for missing non-invitation account types; these are the exact semantics Rust's pre-broadcast durability gate relies on.
source: ['claude', 'codex']
| currentCoroutineContext().ensureActive() | ||
| return withContext(NonCancellable) { | ||
| gate.op { | ||
| val blob = mapNativeErrors { | ||
| IdentityNative.createInvitation( | ||
| walletHandle, | ||
| amountDuffs, | ||
| fundingAccountIndex, | ||
| inviterIdentityId, | ||
| inviterUsername, | ||
| nowUnix, | ||
| coreSignerHandle, | ||
| ) | ||
| } | ||
| // Blob layout (fixed by the JNI): outpoint[36] (txid[32] || vout_le[4]) | ||
| // then the UTF-8 URI. Anything shorter is a contract violation. | ||
| require(blob.size >= 36) { | ||
| "createInvitation returned a ${blob.size}-byte blob; expected >= 36" | ||
| } | ||
| CreatedInvitation( | ||
| outPoint = blob.copyOfRange(0, 36), | ||
| uri = String(blob.copyOfRange(36, blob.size), Charsets.UTF_8), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Regression-test the non-cancellable bearer-URI handoff
The fix correctly keeps the gated JNI call, blob validation, URI decoding, and CreatedInvitation construction inside withContext(NonCancellable), but no createInvitation test or injectable native-call seam exercises cancellation during the blocking call. Add an internal create-invitation native-call dependency, cancel after the fake native call starts, then release it and verify that caller-visible code receives the exact outpoint and URI and that the teardown-gate lease is released. This protects the method-level guarantee from a future refactor that narrows or removes the protected region.
source: ['claude', 'codex']
| * @param amountDuffs voucher amount in duffs (must be positive). | ||
| * @param fundingAccountIndex BIP-44 account the voucher is funded from. | ||
| * Cancellation contract: the caller's cancellation is honored BEFORE the | ||
| * native call starts, but once it begins the operation runs to completion | ||
| * under [NonCancellable] and the result is always delivered. The native op | ||
| * may have broadcast the asset lock and generated the bearer URI by the | ||
| * time cancellation is observed; JNI cannot see Kotlin cancellation, Room | ||
| * intentionally stores no URI or voucher key, and no regeneration API | ||
| * exists — so a discarded `withContext` result would lose the only | ||
| * shareable credential after funds have moved. | ||
| * |
There was a problem hiding this comment.
💬 Nitpick: Move the cancellation contract out of the fundingAccountIndex tag
KDoc treats untagged lines after @param fundingAccountIndex as that parameter's description until the next block tag, so the cancellation contract is rendered as part of fundingAccountIndex instead of the method overview. Move the paragraph above the first @param tag so generated documentation presents it as method-level behavior.
| * @param amountDuffs voucher amount in duffs (must be positive). | |
| * @param fundingAccountIndex BIP-44 account the voucher is funded from. | |
| * Cancellation contract: the caller's cancellation is honored BEFORE the | |
| * native call starts, but once it begins the operation runs to completion | |
| * under [NonCancellable] and the result is always delivered. The native op | |
| * may have broadcast the asset lock and generated the bearer URI by the | |
| * time cancellation is observed; JNI cannot see Kotlin cancellation, Room | |
| * intentionally stores no URI or voucher key, and no regeneration API | |
| * exists — so a discarded `withContext` result would lose the only | |
| * shareable credential after funds have moved. | |
| * | |
| * Cancellation contract: the caller's cancellation is honored BEFORE the | |
| * native call starts, but once it begins the operation runs to completion | |
| * under [NonCancellable] and the result is always delivered. The native op | |
| * may have broadcast the asset lock and generated the bearer URI by the | |
| * time cancellation is observed; JNI cannot see Kotlin cancellation, Room | |
| * intentionally stores no URI or voucher key, and no regeneration API | |
| * exists — so a discarded `withContext` result would lose the only | |
| * shareable credential after funds have moved. | |
| * | |
| * @param amountDuffs voucher amount in duffs (must be positive). | |
| * @param fundingAccountIndex BIP-44 account the voucher is funded from. | |
| * |
source: ['claude']
…orrows `createInvitation` is the SDK's only block-bodied handle-borrowing suspend fun, and it IS gated — the NonCancellable bearer-URI fix (638430b) moved it off an expression body so the lint's `= gate.op {` opener regex stopped matching, failing CI on a correctly-fenced method. Allowlisting it would have been wrong: it is not exempt, and an ALLOWLIST entry would blind the lint to a future refactor that drops the bracket. So the scan now understands both body shapes instead. Expression bodies keep the exact opener rule. A block body must open its bracket before the first JNI call, which still rejects every real defect shape — no bracket, a plain `withContext(Dispatchers.IO)` borrow, a bracket opened after the native call, and a native call leaked outside the bracket — while allowing the `require(...)` preamble and `withContext(NonCancellable)` delivery wrapper that the cancellation contract requires. Verified against the 51 handle-borrowing suspend funs in the source set plus synthetic cases for each violation shape. 186 tests, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt (2)
1368-1390: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject invitation upserts for deleted wallets.
deleteWalletDataLockedremoves invitation rows, and the invitation table has no wallet foreign key. A lateonPersistInvitationUpsertcallback can therefore recreate a deleted row because this path does not verify thatwalletIdstill exists.Check the wallet inside the staged operation. This also protects the new
DataManager.clear(Category.WALLETS)path.Proposed fix
stage(walletId) { db -> + if (db.walletDao().getByWalletId(walletId) == null) return@stage db.invitationDao().upsert(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt` around lines 1368 - 1390, Update onPersistInvitationUpsert so its staged operation verifies that walletId still exists before calling invitationDao().upsert. Reject or no-op the upsert when the wallet has been deleted, while preserving the existing insertion behavior for valid wallets and ensuring compatibility with DataManager.clear(Category.WALLETS).
126-126: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winApply the repository’s two-space indentation rule to the added Kotlin code.
The changed Kotlin blocks use four-space indentation levels. Reindent each touched block to two spaces for non-Rust files.
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt#L126-L126: reindent the capability continuation.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/services/DataManager.kt#L50-L54: reindent the invitation-clear block.packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.kt#L67-L79: reindent the capability assertions.As per coding guidelines, non-Rust files use 2-space indentation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt` at line 126, Reindent the touched Kotlin code to the repository’s two-space style: adjust the capability continuation in packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt lines 126-126, the invitation-clear block in packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/services/DataManager.kt lines 50-54, and the capability assertions in packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.kt lines 67-79. Preserve all code and behavior.Source: Coding guidelines
🧹 Nitpick comments (1)
packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/wallet/GateCoverageLintTest.kt (1)
114-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBrace counting ignores string literals and line comments.
endOfBlockcounts every{and}. A body that contains an unmatched brace inside a string literal or a comment (for example"}"or// }) ends the block early. The truncatedbodycan then hide a real native call or a real gate, so the lint result flips silently. The KDoc covers only${...}interpolation, which is balanced.This is a test-only heuristic, so it is safe to defer. If you want more robustness, skip over line comments and simple string literals while scanning.
♻️ Optional hardening sketch
private fun endOfBlock(src: String, open: Int): Int { var depth = 0 var i = open while (i < src.length) { + // Skip line comments and simple string literals so their braces + // cannot unbalance the count. + if (src.startsWith("//", i)) { + i = src.indexOf('\n', i).let { if (it < 0) src.length else it } + continue + } when (src[i]) { '{' -> depth++ '}' -> { depth-- if (depth == 0) return i + 1 } } i++ } return src.length }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/wallet/GateCoverageLintTest.kt` around lines 114 - 127, Harden the brace scan in endOfBlock so braces inside line comments and simple string literals are ignored while locating the matching closing brace. Preserve normal brace-depth handling for code and the existing fallback return of src.length when no match is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- Around line 1368-1390: Update onPersistInvitationUpsert so its staged
operation verifies that walletId still exists before calling
invitationDao().upsert. Reject or no-op the upsert when the wallet has been
deleted, while preserving the existing insertion behavior for valid wallets and
ensuring compatibility with DataManager.clear(Category.WALLETS).
- Line 126: Reindent the touched Kotlin code to the repository’s two-space
style: adjust the capability continuation in
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt
lines 126-126, the invitation-clear block in
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/services/DataManager.kt
lines 50-54, and the capability assertions in
packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.kt
lines 67-79. Preserve all code and behavior.
---
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/wallet/GateCoverageLintTest.kt`:
- Around line 114-127: Harden the brace scan in endOfBlock so braces inside line
comments and simple string literals are ignored while locating the matching
closing brace. Preserve normal brace-depth handling for code and the existing
fallback return of src.length when no match is found.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 84bbbccd-c0da-4b08-8f9d-f4ba345525c1
📒 Files selected for processing (6)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/services/DataManager.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/wallet/GateCoverageLintTest.ktpackages/rs-platform-wallet/src/wallet/identity/network/invitation.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt
|
@thepastaclaw the review has been showing "Codex precheck starting" at |
2 participants
Summary
Standalone Android counterpart to the DIP-13 invitation flow. This adds the
Kotlin-SDK / JNI bridge for the existing, iOS-shipped
create_invitation/claim_invitationplatform-wallet FFI — no newRust wallet logic; the whole fund / broadcast / persist / proof / export
pipeline already lives in
rs-platform-walletandrs-platform-wallet-ffion
v4.2-devand ships on iOS. This PR just wires Android to it and makesthe required persistence durable.
It is the standalone Android change, not stacked on any other branch.
What's included
JNI bridge (
rs-unified-sdk-jni)Java_..._createInvitation— thin marshaler overplatform_wallet_create_invitation: funds a one-time asset-lock voucherand returns
outpoint[36] || utf8 dashpay://invite URI. The URI embedsthe bearer voucher key and is never logged.
Java_..._claimInvitation— thin marshaler overplatform_wallet_claim_invitation: registers a new identity funded by theimported voucher. Decodes the invitee's key rows via
decode_registration_pubkeys_blob— the same codec pathregisterIdentityWithFundinguses on this base.on_persist_invitations_fn(tramp_persist_invitations), replacingthe previous
None.DIP-13 invitation persistence + capability gate (
kotlin-sdk)IdentityNativeexterns;IdentityRegistration.createInvitation/claimInvitationsuspend wrappers (withCreatedInvitationwhosetoStringredacts the bearer URI).NativePersistenceBridge.onPersistInvitationUpsert/...Removaldispatch;
PlatformWalletPersistenceHandleroverrides that persist to Room.CAPABILITY_INVITATIONS = 0x02added topersistenceCapabilitiesBits().This is load-bearing: the Rust
create_invitationdurability gate refusesto move any funds unless the backend reports the
INVITATIONScapability,which is only true when the upsert callback is genuinely durable. A no-op
store would defeat the gate and risk re-exporting a one-time voucher key
after a restart, so the flow fails closed on a backend that can't record it.
Room storage
invitationstable (InvitationEntity/InvitationDao), one row per36-byte funding outpoint, cascaded on wallet deletion via
deleteWalletData.DashDatabasev7 → v8 withMIGRATION_7_8and exported8.json.Migration-number reconciliation
The change was originally authored against an integration branch whose
DashDatabasewas already at version 8 (an unrelated feature held itsMIGRATION_7_8slot), so it landed there as v8 → v9 /9.json. Onv4.2-devthe current schema is version 7 (MIGRATION_6_7is the lastmigration). The invitation migration has therefore been renumbered to
MIGRATION_7_8(version 8) and the exported schema regenerated as8.json(a fresh Room export from this base — v7 tables +invitations),so there is no collision on
v4.2-dev.Verification
cargo check -p rs-unified-sdk-jni— clean../gradlew :sdk:compileDebugKotlin(JDK17) —BUILD SUCCESSFUL(Room KSP re-exported
8.json).L1 (Standard, asset-lock + islock) and shielded invites created
successfully end-to-end.
Summary by CodeRabbit
New Features
Bug Fixes